feat: add auth + google auth provider for web #6505
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dinhlongviolin1
changed the title
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 9872936 in 1 minute and 57 seconds. Click for details.
- Reviewed
2285lines of code in36files - Skipped
1files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. extensions-web/src/index.ts:12
- Draft comment:
Re-exporting auth functionality and types enhances modularity. Ensure these new exports are documented in the package README for clarity. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is purely informative and suggests ensuring documentation, which violates the rules. It doesn't provide a specific code suggestion or highlight a potential issue with the code itself.
2. extensions-web/src/shared/auth/providers/google.ts:8
- Draft comment:
GoogleAuthProvider implementation is straightforward. Consider future-proofing by handling potential API error cases or adding customization via environment variables if needed. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment violates several rules: it's speculative ("consider future-proofing"), doesn't point to a clear issue requiring changes, and makes assumptions about future needs without evidence. The implementation follows what appears to be a standard pattern for auth providers. Error handling would likely be in BaseAuthProvider if needed. Perhaps there are legitimate concerns about error handling that I'm missing. Maybe environment configuration is a standard requirement for auth providers. Without seeing BaseAuthProvider or evidence that error handling or environment configuration is missing, these suggestions are speculative. The implementation matches the apparent pattern for auth providers. The comment should be deleted as it's speculative, doesn't point to clear issues, and suggests changes without strong evidence they're needed.
3. web-app/src/test/setup.ts:27
- Draft comment:
Authentication is mocked as disabled in tests (authentication: false). Verify that tests not needing auth continue to pass, and consider adding separate tests for enabled auth scenarios. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =50%<= threshold50%The comment suggests verifying that tests continue to pass and adding separate tests for enabled auth scenarios. It is asking the author to ensure tests pass, which violates the rule against asking the author to ensure behavior is intended or tested. However, the suggestion to add separate tests for enabled auth scenarios is specific and could be useful.
4. web-app/src/providers/AuthProvider.tsx:15
- Draft comment:
AuthProvider correctly initializes the auth service asynchronously. Ensure error logging and fallback behavior are sufficient if initialization fails. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that error logging and fallback behavior are sufficient, which violates the rule against asking the author to ensure things. It doesn't provide a specific suggestion or point out a specific issue with the code.
Workflow ID: wflow_l0syS6frHmgWYds1
You can customize 
Contributor
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 5c0845d in 2 minutes and 13 seconds. Click for details.
- Reviewed
176lines of code in4files - Skipped
0files when reviewing. - Skipped posting
7draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. extensions-web/src/index.ts:38
- Draft comment:
Re-exporting ProviderType is clear. Ensure that its definition in './shared/auth' and downstream in providers remains consistent. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. extensions-web/src/shared/auth/index.ts:7
- Draft comment:
Including ProviderType in type exports improves consistency. Verify that its implementation in './providers' aligns with its usage. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. web-app/src/hooks/useAuth.ts:75
- Draft comment:
Switching loginWithProvider to accept ProviderType enhances type safety. Ensure that literal strings like 'google' conform to ProviderType’s definition. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. web-app/src/hooks/useAuth.ts:84
- Draft comment:
handleProviderCallback now uses ProviderType. Confirm that the underlying authService.handleProviderCallback method properly accepts this type. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. web-app/src/hooks/useAuth.ts:99
- Draft comment:
isAuthenticatedWithProvider now accepts ProviderType, ensuring consistent provider identification across methods. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. web-app/src/routes/auth.google.callback.tsx:25
- Draft comment:
The async callback in useEffect is well-structured. Consider adding a cleanup mechanism or cancellation check to handle potential unmounts during async operations. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
7. web-app/src/routes/auth.google.callback.tsx:52
- Draft comment:
Ensure that using the literal 'google' as the ProviderType is valid per the provider registry's definitions. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_gj9gqUbODppUxmfJ
You can customize
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed f1939f8 in 1 minute and 23 seconds. Click for details.
- Reviewed
30lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/containers/auth/AuthLoginButton.tsx:11
- Draft comment:
Importing ProviderType is appropriate for type checking. Ensure that the module provides the expected type shape. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
2. web-app/src/containers/auth/AuthLoginButton.tsx:26
- Draft comment:
Changing the parameter type from string to ProviderType is good. Verify that getEnabledProviders returns an object with an id matching ProviderType to avoid unnecessary casts. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
3. web-app/src/containers/auth/AuthLoginButton.tsx:68
- Draft comment:
Casting provider.id to ProviderType works, but consider refining the provider type so that an explicit cast isn't needed. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
Workflow ID: wflow_Ydx4mMlLywapoIAL
You can customize
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed bf009b1 in 1 minute and 54 seconds. Click for details.
- Reviewed
125lines of code in8files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/containers/auth/UserProfileMenu.tsx:76
- Draft comment:
Updated DropdownMenuContent props: using side='right' and align='start'. Verify that this change meets the intended UI positioning across device sizes. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify the UI positioning across device sizes, which falls under the rule of not asking the author to ensure behavior is intended or to double-check things. This is not a specific code suggestion or a request for a test to be written, so it should be removed.
2. web-app/src/locales/en/common.json:30
- Draft comment:
New auth-related localization keys (logout, loggingOut, loggedOut, logoutFailed, profile) have been added. Confirm these keys are consistently referenced in the UI and that similar updates across other locale files remain in sync. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm that the new localization keys are consistently referenced and that updates across locale files are in sync. This falls under asking the author to double-check things, which is against the rules.
3. web-app/src/locales/de-DE/common.json:34
- Draft comment:
Typographical suggestion: The text for "loggingOut" is currently "Melde ab...". For consistency with the imperative tone used in "login" ("Anmelden") and "logout" ("Abmelden"), consider changing it to "Abmelden...". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% This is a translation file and the comment points out an actual inconsistency in the German translations. The suggestion would make the translations more consistent in their grammatical form. However, this is primarily a UI/translation issue rather than a logic issue. Per the rules, we should not comment on pure UI changes as we should assume the author made UI choices correctly. While the comment identifies a real inconsistency, it's fundamentally about UI text and translation choices, which the rules explicitly tell us to stay out of. Even though the suggestion is technically correct, we must follow the rule about not commenting on UI changes. Translation choices should be left to the PR author. Delete this comment since it relates to UI text/translation choices, which we're instructed not to comment on.
Workflow ID: wflow_uaUOpqCe2bqAmKU6
You can customize
Contributor
Author
|
@jjchen01 thanks for helping me test it! |
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 02e3b8f in 1 minute and 37 seconds. Click for details.
- Reviewed
39lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/containers/auth/AuthLoginButton.tsx:21
- Draft comment:
Renamed getEnabledProviders to getAllProviders. For consistency, consider renaming the local variable 'enabledProviders' on line 24 to 'providers'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While the comment is technically correct about the inconsistency, variable naming suggestions like this are very minor and don't impact functionality. The current variable name enabledProviders is still semantically meaningful, as it presumably stores providers that are enabled/available for use. The comment doesn't point out any actual issues with the code's behavior or structure. The variable name inconsistency could potentially cause confusion for future developers about whether there's a meaningful difference between "all" providers and "enabled" providers. However, this is a very minor stylistic issue. If there was a meaningful distinction between "all" and "enabled" providers, it would be caught during testing or code review of the actual functionality. This comment should be deleted as it addresses a minor naming inconsistency that doesn't impact code functionality or maintainability significantly.
2. web-app/src/hooks/useAuth.ts:25
- Draft comment:
Updated the provider function name to getAllProviders; ensure related comments/documentation are updated accordingly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that related comments/documentation are updated, which violates the rule against asking the author to ensure things. It doesn't provide a specific suggestion or point out a specific issue with the code.
Workflow ID: wflow_qEVXJGDeVrXB5oFZ
You can customize
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe Your Changes
Fixes Issues
Self Checklist
Important
Add Google OAuth authentication to the web app with new UI components, routing, and localization support.
JanAuthServiceinextensions-web/src/shared/auth/service.tsfor handling OAuth flows.extensions-web/src/shared/auth/providers/google.ts.AuthProvidercomponent inweb-app/src/providers/AuthProvider.tsxto initialize auth service.AuthLoginButtonandUserProfileMenucomponents for login/logout UI inweb-app/src/containers/auth/.useAuthhook inweb-app/src/hooks/useAuth.tsfor managing auth state./auth/google/callbackroute inweb-app/src/routes/auth.google.callback.tsxfor handling OAuth callbacks.routeTree.gen.tsto include new auth route.AvatarandCardcomponents inweb-app/src/components/ui/for user profile display.web-app/src/locales/to include auth-related strings.PlatformFeaturesinweb-app/src/lib/platform/const.tsto enable authentication for web.@radix-ui/react-avatartoweb-app/package.jsondependencies.This description was created by
for 02e3b8f. You can customize this summary. It will automatically update as commits are pushed.